-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add normalized CPU values and number of cores #4553
Conversation
metricbeat/metricbeat.full.yml
Outdated
# if true, exports the CPU usage in ticks, together with the percentage values | ||
#cpu_ticks: false | ||
# Configure the metric types that are included by these metricsets. | ||
cpu.metrics: [percentages] # The other available options are normalized_percentages and ticks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add "quotes" around percentages? Seems we do this everywhere else with strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the quotes for consistency. 3b23220
"idle": { | ||
"pct": 0.9063, | ||
"ticks": 22204290 | ||
"pct": 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you run it 2-3 more times, you should also get some values for the pct
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 96a5b91
// Config for the system core metricset. | ||
type Config struct { | ||
Metrics []string `config:"core.metrics"` | ||
CPUTicks *bool `config:"cpu_ticks"` // Deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecate it in 5.6 and remove it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruflin Any reason to deprecate it in 5.6 instead of 6.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like deprecated features in the code base. If don't do it now, we have to wait until 7.0 to remove it. Seems like a good time to do it now. Any disadvantage doing it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to add deprecation warnings in 5.6 when there is no replacement for cpu_ticks in 5.6? I thought there should be some period of overlap where the two config options are available to aid in migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean no replacement in 6.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant 5.6. But possibly I misunderstood the original suggestion. You were suggesting adding the deprecation warnings in 5.6 and not having a migration path in 5.6 (e.g. we are telling the user not to use cpu_ticks
, but not offering them an alternative in 5.6)?
// Validate validates the cpu config. | ||
func (c Config) Validate() error { | ||
if c.CPUTicks != nil { | ||
logp.Deprecate("6.0.0", "cpu_ticks is deprecated. Add 'ticks' to the cpu.metrics list.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
Merged #4550, can you rebase this one, please? The flaky container failures in Metricbeat/travis should also be fixed after rebase. |
Remove the arguments and use a constant to control the maximum number of decimal places.
e86be9d
to
84e718f
Compare
I rebased the PR to resolve the conflicts, and squashed the commits where I addressed action items because I think it would be best to "Rebase and merge" this PR. |
- Added normalized CPU metrics for core, cpu, and process metricsets that are divided by the number of CPU cores. - Refactored helper code used by core, cpu, and load metricsets. - Deprecated use of `cpu_ticks` in favor of per metricset settings.
84e718f
to
979091f
Compare
This include the contents of #4550 (so merge that first and the amount to review will be smaller).
cpu_ticks
in favor of per metricset settings.